Skip to content

Night Shift: fix broken Convex filter in toggleLineLearned#18

Open
EtanHey wants to merge 1 commit intomasterfrom
nightshift/2026-02-22-5637
Open

Night Shift: fix broken Convex filter in toggleLineLearned#18
EtanHey wants to merge 1 commit intomasterfrom
nightshift/2026-02-22-5637

Conversation

@EtanHey
Copy link
Owner

@EtanHey EtanHey commented Feb 22, 2026

User description

Automated improvement by Golems Night Shift.

fix broken Convex filter in toggleLineLearned


PR Type

Bug fix


Description

  • Fixed broken Convex filter using q.and() instead of JavaScript &&

  • Prevents toggling wrong line's learned state across multiple songs

  • Ensures both songId and lineNumber conditions are properly combined


Diagram Walkthrough

flowchart LR
  A["JavaScript && operator"] -->|"silently drops left operand"| B["Broken filter logic"]
  B -->|"toggles wrong line"| C["Bug: incorrect state change"]
  D["q.and() combinator"] -->|"properly combines conditions"| E["Fixed filter logic"]
  E -->|"toggles correct line"| F["Fix: correct state change"]
Loading

File Walkthrough

Relevant files
Bug fix
songProgress.ts
Replace && with q.and() in filter logic                                   

convex/songProgress.ts

  • Replaced JavaScript && operator with q.and() in toggleLineLearned
    filter
  • Properly combines songId and lineNumber filter conditions
  • Prevents silent dropping of the songId filter condition
+4/-2     

Note

Low Risk
Small query predicate change in a single Convex mutation; low blast radius but could affect whether the correct lineProgress record is found/toggled.

Overview
Fixes toggleLineLearned in convex/songProgress.ts by updating the lineProgress lookup filter to use q.and(...) to combine songId and lineNumber predicates.

This prevents the mutation from using an invalid/broken filter expression, ensuring the correct per-line progress record is found and toggled.

Written by Cursor Bugbot for commit 1a46a1d. This will update automatically on new commits. Configure here.

Summary by CodeRabbit

  • Refactor
    • Optimized internal query filtering mechanisms to enhance system performance and efficiency. The refinement streamlines how data filtering conditions are processed, resulting in improved query performance and better resource utilization throughout the system, while maintaining complete backward compatibility with all existing features and functionality.

The filter used JavaScript && between Convex query expressions:
  q.eq(q.field("songId"), ...) && q.eq(q.field("lineNumber"), ...)

Since && returns the right operand when the left is truthy (any object),
this silently dropped the songId filter. The mutation could toggle the
wrong line's learned state when a user has multiple songs with matching
line numbers.

Fixed by using q.and() to properly combine both filter conditions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@vercel
Copy link

vercel bot commented Feb 22, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
songscript Ready Ready Preview, Comment Feb 22, 2026 2:06am

@coderabbitai
Copy link

coderabbitai bot commented Feb 22, 2026

📝 Walkthrough

Walkthrough

The toggleLineLearned function in songProgress.ts is modified to consolidate its filter condition from two separate equality checks into a single combined AND condition for filtering by songId and lineNumber.

Changes

Cohort / File(s) Summary
Filter Logic Consolidation
convex/songProgress.ts
Modified the observable filter in toggleLineLearned to use a combined AND condition instead of two separate equality checks when filtering by songId and lineNumber.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A filter refined, now sleek and clean,
Two checks become one, sharp and lean,
The query flows smoother through database dreams,
Where logic converges in elegant streams!

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly references fixing a broken Convex filter in toggleLineLearned, which matches the main change described in the PR objectives.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch nightshift/2026-02-22-5637

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-code-review
Copy link

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status:
Missing audit logging: The toggleLineLearned mutation performs a write that may be considered a critical user
action, but no audit log emission is visible in the diff to confirm user/action/outcome
are recorded.

Referred Code
export const toggleLineLearned = mutation({
  args: {
    songId: v.id("songs"),
    lineNumber: v.number()
  },
  handler: async (ctx, args) => {
    const userId = await requireAuth(ctx);

    const existing = await ctx.db
      .query("lineProgress")
      .withIndex("by_user", (q) =>
        q.eq("userId", userId)
      )
      .filter((q) =>
        q.and(
          q.eq(q.field("songId"), args.songId),
          q.eq(q.field("lineNumber"), args.lineNumber)
        )
      )
      .first();



 ... (clipped 1 lines)

Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-code-review
Copy link

PR Code Suggestions ✨

No code suggestions found for the PR.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
convex/songProgress.ts (2)

233-244: ⚠️ Potential issue | 🟠 Major

Add composite index on (userId, songId) to lineProgress table to avoid full per-user scans.

The current query uses withIndex("by_user") which fetches all of a user's lineProgress rows regardless of song, then filters songId and lineNumber in memory. As a user learns more lines across more songs, this scan grows unboundedly. The schema has composite indexes for visitorId combinations but not for userId. Adding an index on (userId, songId) — or (userId, songId, lineNumber) for a point-lookup — would reduce this to O(lines in song) or O(1).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@convex/songProgress.ts` around lines 233 - 244, The query on lineProgress
using withIndex("by_user") performs a full per-user scan and then filters songId
and lineNumber in memory; add a composite index on (userId, songId) or (userId,
songId, lineNumber) to the lineProgress table schema so the query becomes
index-backed (update the schema/index definitions for lineProgress to include an
index keyed by userId+songId or userId+songId+lineNumber) and then switch the
query to use that index (replace withIndex("by_user") with the new index name)
so lookups by userId/songId/(lineNumber) are efficient.

225-303: ⚠️ Potential issue | 🟡 Minor

Add a regression test for the toggleLineLearned mutation to verify multi-song lineNumber collision is prevented.

The mutation's fix correctly filters by both songId and lineNumber combined, but this critical logic lacks test coverage. Without a targeted test, the bug—where toggling a line in one song could affect another song with the same line number—could silently regress. Add a Convex integration test that verifies toggling line N in song A does not affect song B's line N.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@convex/songProgress.ts` around lines 225 - 303, Add a Convex integration
regression test for the toggleLineLearned mutation that ensures toggling a line
for one song does not affect another song with the same lineNumber: set up two
distinct songs (songA, songB), call toggleLineLearned with songA and lineNumber
N, then query the lineProgress and userSongProgress collections to assert a
lineProgress/userSongProgress was created/updated for songA but no lineProgress
or song progress exists or was modified for songB; reference the mutation name
toggleLineLearned and the collections/records "lineProgress" and
"userSongProgress" when locating where to add the assertions.
📜 Review details

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9a964b0 and 1a46a1d.

📒 Files selected for processing (1)
  • convex/songProgress.ts
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx}

📄 CodeRabbit inference engine (AGENTS.md)

Use TanStack Start framework with Bun runtime for the application

Tests must pass locally via bun run test before committing code, as Husky pre-commit hooks will block commits with failing tests

Files:

  • convex/songProgress.ts
convex/*.ts

📄 CodeRabbit inference engine (AGENTS.md)

Use Convex for database queries, mutations, and authentication configuration

Files:

  • convex/songProgress.ts
**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

All new helpers and utilities MUST have corresponding test files

Files:

  • convex/songProgress.ts
convex/**/*.ts

📄 CodeRabbit inference engine (CLAUDE.md)

convex/**/*.ts: NEVER create .js files in the convex/ folder - only .ts files belong there
Before starting Convex dev server, always run rm -f convex/*.js to clean compiled JavaScript files
Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations

Files:

  • convex/songProgress.ts
🧠 Learnings (3)
📚 Learning: 2026-01-28T12:17:53.877Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-28T12:17:53.877Z
Learning: Applies to convex/**/*.ts : Use Convex schema.ts for database schema definitions and songs.ts for queries and mutations

Applied to files:

  • convex/songProgress.ts
📚 Learning: 2026-01-23T18:12:49.193Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: prd-json/AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:49.193Z
Learning: Applies to prd-json/**/convex/**/*{score,progress,leaderboard}*.{ts,tsx,js} : Use Progress Score Formula: (words_learned × multiplier) + (lines_completed × multiplier × 0.5)

Applied to files:

  • convex/songProgress.ts
📚 Learning: 2026-01-23T18:12:38.519Z
Learnt from: CR
Repo: EtanHey/songscript PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-23T18:12:38.519Z
Learning: Applies to convex/*.ts : Use Convex for database queries, mutations, and authentication configuration

Applied to files:

  • convex/songProgress.ts
🔇 Additional comments (1)
convex/songProgress.ts (1)

238-243: Fix is correct — q.and() is the right Convex API here.

The old && silently discarded the songId predicate because Convex filter expressions are objects (always truthy), so objA && objB returns objB. Replacing it with q.and() correctly passes both predicates to the Convex query engine.

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@convex/songProgress.ts`:
- Around line 233-244: The query on lineProgress using withIndex("by_user")
performs a full per-user scan and then filters songId and lineNumber in memory;
add a composite index on (userId, songId) or (userId, songId, lineNumber) to the
lineProgress table schema so the query becomes index-backed (update the
schema/index definitions for lineProgress to include an index keyed by
userId+songId or userId+songId+lineNumber) and then switch the query to use that
index (replace withIndex("by_user") with the new index name) so lookups by
userId/songId/(lineNumber) are efficient.
- Around line 225-303: Add a Convex integration regression test for the
toggleLineLearned mutation that ensures toggling a line for one song does not
affect another song with the same lineNumber: set up two distinct songs (songA,
songB), call toggleLineLearned with songA and lineNumber N, then query the
lineProgress and userSongProgress collections to assert a
lineProgress/userSongProgress was created/updated for songA but no lineProgress
or song progress exists or was modified for songB; reference the mutation name
toggleLineLearned and the collections/records "lineProgress" and
"userSongProgress" when locating where to add the assertions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant